Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Translate _ as __ #1008

Merged
merged 1 commit into from
Sep 21, 2017
Merged

Conversation

AndrewGaspar
Copy link
Contributor

@AndrewGaspar AndrewGaspar commented Sep 21, 2017

This change treats _ as a reserved identifier to resolve the bug reported in #631.

I have one concern - if the header has both an _ and __ identifier in the global namespace, this will cause a conflict. However, it seems like we already don't handle that case for keyword_ (e.g. abstract_, alignof_, etc.) so it doesn't seem like we need a solution specifically for __ in this change.

Fixes #631.

@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 thanks @AndrewGaspar !

@fitzgen
Copy link
Member

fitzgen commented Sep 21, 2017

I have one concern - if the header has both an _ and __ identifier in the global namespace, this will cause a conflict. However, it seems like we already don't handle that case for keyword_ (e.g. abstract_, alignof_, etc.) so it doesn't seem like we need a solution specifically for __ in this change.

We attempt to handle this sort of thing during the codegen phase, and have a map of method names and field names where we count how many times a name has been seen, and append that number to the name for disambiguation. So you get foo, foo1, foo2, foo3. etc.

It isn't perfect, but... shrug

@fitzgen
Copy link
Member

fitzgen commented Sep 21, 2017

@bors-servo r+

@bors-servo
Copy link

📌 Commit aa11524 has been approved by fitzgen

@bors-servo
Copy link

⌛ Testing commit aa11524 with merge 5b49548...

bors-servo pushed a commit that referenced this pull request Sep 21, 2017
Translate _ as __

This change treats _ as a reserved identifier to resolve the bug reported in #631.

I have one concern - if the header has both an `_` and `__` identifier in the global namespace, this will cause a conflict. However, it seems like we already don't handle that case for `keyword_` (e.g. `abstract_`, `alignof_`, etc.) so it doesn't seem like we need a solution specifically for `__` in this change.

Fixes #631.
@bors-servo
Copy link

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing 5b49548 to master...

@bors-servo bors-servo merged commit aa11524 into rust-lang:master Sep 21, 2017
@AndrewGaspar AndrewGaspar deleted the underscore-identifier branch September 22, 2017 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bindgen generates erroneous rust code: error: expected identifier, found _
4 participants